-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix ensure_monomorphic_enough #136839
fix ensure_monomorphic_enough #136839
Conversation
rustbot has assigned @petrochenkov. Use |
Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval |
r? @saethlin |
I don't think that's a fair characterization, considering that what is apparently a soundness hole made it through code review and almost to stable. I think we have plenty of regression tests already which the same characterization would apply to. I would rather have the test. |
63ecba7
to
c1da4f1
Compare
Fair. I'm not trying to throw shade here.
Sure, added a test. |
I do think it's obviously wrong, but also you may want to choose your words more carefully next time 😃 Things that seem obviously wrong in hindsight are often overlooked. In this case, I probably just mis-reasoned when reviewing about the polarity of the @bors r+ rollup This is a simple backport, should be relatively uncontroversial but also should be done before stable artifacts are prepared soon for 1.85. @rustbot label: beta-nominated |
…ough, r=compiler-errors fix ensure_monomorphic_enough When polymorphization was still a thing, the visitor was used to only recurse into *used generic parameters* of function/closure/coroutine types and allow unused parameters (i.e. the polymorphized parameters) to remain generic. When polymorphization got removed, this got changed to always treat all parameters as polymorphic and never recurse into them: /~https://github.com/rust-lang/rust/pull/133883/files#diff-210c59e321070d0ca4625c04e9fb064bf43ddc34082e7e33a7ee8a6c577e95afL44-L62 This is clearly wrong and can cause MIR opts to misbehave, for example this currently prints "false" in release mode: ```rust #![feature(core_intrinsics)] fn generic<T>() {} const fn type_id_of_val<T: 'static>(_: &T) -> u128 { std::intrinsics::type_id::<T>() } fn cursed_is_i32<T: 'static>() -> bool { (const { type_id_of_val(&generic::<T>) } == const { type_id_of_val(&generic::<i32>) }) } fn main() { dbg!(cursed_is_i32::<i32>()); } ``` This PR reverts to the old behavior of always treating all types that contain type parameters as too generic, like we used to do without `-Zpolymorphize` before. ~~I'm not including the above as a test case here, because I think there is little value in testing code paths that have been removed and this seems unlikely to regress in a way that would be caught by a regression test, but let me know if you disagree and want me to add a test anyway.~~
Which soundness hole are you referring to here? |
Rollup of 11 pull requests Successful merges: - rust-lang#136606 (Fix long lines which rustfmt fails to format) - rust-lang#136663 (Stabilize `NonZero::count_ones`) - rust-lang#136672 (library: doc: core::alloc::Allocator: trivial typo fix) - rust-lang#136704 (Improve examples for file locking) - rust-lang#136721 (cg_llvm: Reduce visibility of some items outside the `llvm` module) - rust-lang#136813 (rustc_target: Add the fp16 target feature for AArch32) - rust-lang#136830 (fix i686-unknown-hurd-gnu x87 footnote) - rust-lang#136832 (Fix platform support table for i686-unknown-uefi) - rust-lang#136835 (Stop using span hack for contracts feature gating) - rust-lang#136837 (Overhaul how contracts are lowered on fn-like bodies) - rust-lang#136839 (fix ensure_monomorphic_enough) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#136839 - lukas-code:actually-monomorphic-enough, r=compiler-errors fix ensure_monomorphic_enough When polymorphization was still a thing, the visitor was used to only recurse into *used generic parameters* of function/closure/coroutine types and allow unused parameters (i.e. the polymorphized parameters) to remain generic. When polymorphization got removed, this got changed to always treat all parameters as polymorphic and never recurse into them: /~https://github.com/rust-lang/rust/pull/133883/files#diff-210c59e321070d0ca4625c04e9fb064bf43ddc34082e7e33a7ee8a6c577e95afL44-L62 This is clearly wrong and can cause MIR opts to misbehave, for example this currently prints "false" in release mode: ```rust #![feature(core_intrinsics)] fn generic<T>() {} const fn type_id_of_val<T: 'static>(_: &T) -> u128 { std::intrinsics::type_id::<T>() } fn cursed_is_i32<T: 'static>() -> bool { (const { type_id_of_val(&generic::<T>) } == const { type_id_of_val(&generic::<i32>) }) } fn main() { dbg!(cursed_is_i32::<i32>()); } ``` This PR reverts to the old behavior of always treating all types that contain type parameters as too generic, like we used to do without `-Zpolymorphize` before. ~~I'm not including the above as a test case here, because I think there is little value in testing code paths that have been removed and this seems unlikely to regress in a way that would be caught by a regression test, but let me know if you disagree and want me to add a test anyway.~~
[beta] backports - Pattern Migration 2024: try to suggest eliding redundant binding modifiers rust-lang#136577, rust-lang#136857 - chore: update rustc-hash 2.1.0 to 2.1.1 rust-lang#136605 - Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]` rust-lang#136724 - fix ensure_monomorphic_enough rust-lang#136839 - Revert "Stabilize `extended_varargs_abi_support`" rust-lang#136897, rust-lang#136934 r? cuviper
When polymorphization was still a thing, the visitor was used to only recurse into used generic parameters of function/closure/coroutine types and allow unused parameters (i.e. the polymorphized parameters) to remain generic.
When polymorphization got removed, this got changed to always treat all parameters as polymorphic and never recurse into them: /~https://github.com/rust-lang/rust/pull/133883/files#diff-210c59e321070d0ca4625c04e9fb064bf43ddc34082e7e33a7ee8a6c577e95afL44-L62
This is clearly wrong and can cause MIR opts to misbehave, for example this currently prints "false" in release mode:
This PR reverts to the old behavior of always treating all types that contain type parameters as too generic, like we used to do without
-Zpolymorphize
before.I'm not including the above as a test case here, because I think there is little value in testing code paths that have been removed and this seems unlikely to regress in a way that would be caught by a regression test, but let me know if you disagree and want me to add a test anyway.